Add function 'decktape()' #177

pat-s · 2018-10-24T20:58:56Z

fixes #168

Long wait. I tried various docker packages to make this function a bit more flexible.
None really worked (or my Docker knowledge is too small to properly deal with the implementations). So in the end I use a simple system() call.

However, the chosen hardcoded arguments should be ok for all users. That's why I did not add an option to change them. If errors are thrown some day with these defaults we need to change them anyways.

I am not sure about the implementation of the tests - I am used to testthat and couldn't really get a test run going with the testit pkg. Feel free to make changes yourself or just tell me what to do.

I added an convenience argument open_pdf to automatically open the resulting PDF file. It is cross-platform and works reliably.

Furthermore I tidied the DESCRIPTION using usethis::use_tidy_description(). Feel free to revert if you don't like it.

Should I add a slide to the demo slides with information about this function?

Merge branch 'master' into pdf # Conflicts: # DESCRIPTION

yihui

I'm fine with a system() wrapper function if the two dependencies are removed, although I still haven't become a fan of Docker. Perhaps we could let users choose whether to use Docker or not (in the latter case, it'd be a system() call to decktape directly).

For testing, the problem is not testthat or testit (the usage of testit should be straightforward enough: https://github.com/yihui/testit), but how to test PDF output. Do we put a copy of PDF output in this package, and compare MD5 sum? That may not be robust. I'd probably just not add tests for this function, since I feel it would be tricky.

Thanks!

yihui · 2018-10-24T21:56:15Z

DESCRIPTION

+    rstudioapi,
+    testit
+VignetteBuilder: 
+    knitr


I hope cosmetic changes can be sent in separate PRs if you really want to make such changes. In general, please try to do one thing per PR. And in particular, cosmetic changes in PRs often become red herrings and can lead to bikeshedding...

can lead to bikeshedding

Yes, that's so true. And prob one of my personal biggest problems to always do such things in a PR while I'm "on something".

Will revert.

yihui · 2018-10-24T21:58:02Z

R/render.R

+                      open_pdf = FALSE) {
+
+  if (is.null(decktape_version)) {
+    system(glue::glue("docker run --rm -t -v `pwd`:/slides -v $HOME:$HOME astefanutti/decktape {xaringan_path} {pdf_path}"))


I don't see the need to introduce the dependency on the glue package. A simple call tosprintf() should suffice.

Yes, there are ofc a lot of functions that do similar stuff. I was just used to glue so I used it.

Will change it.

yihui · 2018-10-24T22:00:24Z

R/render.R

+  }
+
+  if (isTRUE(open_pdf)) {
+    PBSmodelling::openFile(pdf_path)


Another (soft) dependency introduced (which further depends on tcltk and XML). I'd rather not have this feature of automatically opening the PDF. It is costly with a relatively small gain. If you really like this feature, I'd just borrow the simple code from animation:::auto_browse.

Ok, then I remove it again. I like it and used this pkg because I know that it works reliably cross-platform.

Tbh I didn't check for the deps it introduces.

pat-s · 2018-10-25T09:55:17Z

I'm fine with a system() wrapper function if the two dependencies are removed, although I still haven't become a fan of Docker. Perhaps we could let users choose whether to use Docker or not (in the latter case, it'd be a system() call to decktape directly).

I think the docker implementation is the big advantage here. It works OS-agnostic while the behavior of a locally installed decktape highly differs. E.g. on some OS you need to use --no-sandbox, on some not. On some OS specific versions of decktape will work fine while on other OS its different.

Also, with docker, you can use multiple versions easily while having multiple local versions of decktape is tedious.

I think a function that simply does a system() call to a local decktape installation even introduces overhead compared to simply calling decktape directly from the terminal.
By this I mean I am faster typing decktape .html .pdf (or evensystem(decktape .html .pdf) in an R workflow) in the terminal (if I have decktape installed locally) than export_pdf(html, pdf, local = TRUE) or similar.
Also, if we implement the "local method" I feat we might get a lot of issues of people facing problems with their local decktape installation. By only using the docker implementation we could state this in the help file and force the user to try multiple versions (usually 2.8.0 works fine for example).

For the docker image an explicit export_pdf() fun makes sense since I never want to remember all these docker arguments that are needed to call decktape.
In the end, the idea of this function is to not have the requirement of having decktape installed locally but just to have a working docker installation.
Well, now one can discuss which case is easier - having a running docker installation or decktape 😆

For testing, the problem is not testthat or testit (the usage of testit should be straightforward enough: yihui/testit), but how to test PDF output. Do we put a copy of PDF output in this package, and compare MD5 sum? That may not be robust. I'd probably just not add tests for this function, since I feel it would be tricky.

I also thought about a hash comparison but yeah - probably we do not need to test this. It's not a crucial function (convenience only) and error messages could be redirected to the decktape repo as its not within our responsibility.

Ok, this now almost becomes a blog post 🙃
Sorry for stealing so much of your time for such a small contribution.

PBSmodelling -> animation

pat-s · 2018-11-01T10:04:47Z

Decided to go with animation:::auto_browse() to open the PDF.
used roxygen markdown notation (hope that's ok as a PR side-effect)
sprintf() instead of glue()
added sections to the documentation

yihui

I'd rename the function to decktape(); export_pdf sounds too general. Thanks!

yihui · 2018-11-01T14:11:28Z

R/render.R

+  }
+
+  if (isTRUE(open_pdf)) {
+    animation:::auto_browse(pdf_path)


Feel free to copy the full source of auto_browse here to avoid dependency on animation.

yihui · 2018-11-01T14:15:04Z

R/render.R

+                   xaringan_path, pdf_path))
+  } else {
+    system(sprintf("docker run --rm -t -v `pwd`:/slides -v $HOME:$HOME astefanutti/decktape:%s %s %s",
+                   decktape_version, xaringan_path, pdf_path))


Are you able to merge the two system() calls into one to avoid the repetition of docker run --rm -t -v pwd:/slides -v $HOME:$HOME astefanutti/decktape?

BTW, what do `pwd`:/slides and $HOME:$HOME mean? Will they work on Windows?

Are you able to merge the two system() calls into one to avoid the repetition of docker run --rm -t -v pwd:/slides -v $HOME:$HOME astefanutti/decktape?

Yes, did so.

BTW, what do pwd:/slides and $HOME:$HOME mean? Will they work on Windows?

AFAICT yes. The two parts you mentioned are required for some local writing permissions for docker. I do not know more details unfortunately. I modified the defaults of the decktape readme because they were not as robust as this notation.

yihui · 2018-11-01T14:15:52Z

R/render.R

+#'
+#' @export
+
+export_pdf = function(xaringan_path = NULL, pdf_path = NULL,


If xaringan_path and pdf_path are required arguments, they shouldn't default to NULL or have defaults at all.

Right, removed the defaults.

yihui · 2018-11-01T14:18:09Z

R/render.R

+#' @section Docker:
+#' To run this function you need to have a working installation of
+#' [docker](https://www.docker.com/). For some operating systems you may need
+#' to [add yourself to the "docker" group](https://stackoverflow.com/questions/48957195/how-to-fix-docker-got-permission-denied-issue?noredirect=1&lq=1)


The link could be shortened to https://stackoverflow.com/questions/48957195.

yihui · 2018-11-01T14:23:49Z

.travis.yml

+    packages:
+      - libgmp-dev
+      - libgsl0-dev
+      - libmagick++-dev # for animation


Shouldn't be necessary after the animation dependency is removed.

yihui · 2018-11-01T14:25:58Z

DESCRIPTION

 License: MIT + file LICENSE
 URL: https://github.com/yihui/xaringan
 BugReports: https://github.com/yihui/xaringan/issues
 VignetteBuilder: knitr
 Encoding: UTF-8
 RoxygenNote: 6.1.0
+Roxygen: list(markdown = TRUE)


Okay this time (you were essentially betting, though).

Hm, sorry. Reverted ;)

pat-s · 2018-11-08T16:25:22Z

@yihui ready for the next (hopefully last) round :)

yihui · 2018-11-09T05:28:58Z

@pat-s Thanks a lot! I'll review and merge it tomorrow.

…that users don't have to install Docker

pat-s added 7 commits October 24, 2018 22:39

add PBSmodelling to suggests, tidy

776fcdb

glue = suggests

67537e2

NS

fa04a95

add export_pdf

bfc08c0

add test

cb6378c

merge master

c113564

Merge branch 'master' into pdf # Conflicts: # DESCRIPTION

update .Rproj

2a05698

pat-s added enhancement PDF export labels Oct 24, 2018

re-add license

0b71a5f

yihui reviewed Oct 24, 2018

View reviewed changes

pat-s added 5 commits November 1, 2018 10:46

glue -> sprintf

c72aef0

PBSmodelling -> animation

update doc

37cde02

use roxygen markdown, revert to old DESC structure

75341d1

delete test

f5219e9

Update xaringan.Rproj

f7818bc

pat-s added 2 commits November 1, 2018 11:08

install libmagick on travis

fc63852

Merge branch 'pdf' of github.com:yihui/xaringan into pdf

db81ec6

yihui requested changes Nov 1, 2018

View reviewed changes

pat-s added 9 commits November 3, 2018 11:58

import auto_browse from animation pkg

c17decc

no defaults for xaringan_path and pdf_path

f9e3075

only one system() call to docker

6360ae2

shorten SO link

1f42a0d

remove magick dep from travis

ba7947a

export_pdf -> decktape()

eb51868

revert roxygen markdown change

b9546a6

update man & NS

0f0e298

remove animation from suggests

e21723d

fix auto_browse()

7054652

also support the local decktape library if it has been installed, so …

ae71743

…that users don't have to install Docker

yihui added this to the v0.8 milestone Nov 9, 2018

yihui added 2 commits November 9, 2018 13:59

add a news item for #177

1427413

note that file could be a URL when not using Docker

1df2d4f

yihui approved these changes Nov 9, 2018

View reviewed changes

yihui merged commit 57a41b5 into master Nov 9, 2018

yihui deleted the pdf branch November 9, 2018 20:09

yihui changed the title ~~Add function 'export_pdf()'~~ Add function 'decktape()' Nov 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add function 'decktape()' #177

Add function 'decktape()' #177

pat-s commented Oct 24, 2018

yihui left a comment

yihui Oct 24, 2018

pat-s Oct 25, 2018

yihui Oct 24, 2018

pat-s Oct 25, 2018

yihui Oct 24, 2018

pat-s Oct 25, 2018

pat-s commented Oct 25, 2018

pat-s commented Nov 1, 2018

yihui left a comment

yihui Nov 1, 2018

pat-s Nov 3, 2018

yihui Nov 1, 2018

pat-s Nov 3, 2018

yihui Nov 1, 2018

pat-s Nov 3, 2018

yihui Nov 1, 2018

pat-s Nov 3, 2018

yihui Nov 1, 2018 •

edited

Loading

pat-s Nov 3, 2018

yihui Nov 1, 2018

pat-s Nov 3, 2018

pat-s commented Nov 8, 2018

yihui commented Nov 9, 2018

Add function 'decktape()' #177

Add function 'decktape()' #177

Conversation

pat-s commented Oct 24, 2018

yihui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pat-s commented Oct 25, 2018

pat-s commented Nov 1, 2018

yihui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihui Nov 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pat-s commented Nov 8, 2018

yihui commented Nov 9, 2018

yihui Nov 1, 2018 •

edited

Loading